chore(protect-ffi): add integration suite to pnpm workspace - #948
Merged
Conversation
`packages/protect-ffi/integration-tests` sat outside the pnpm workspace with its own `package-lock.json`, installed by `npm ci`. That made it the last place in the tree where `@cipherstash/eql` was pinned to a published version — and it is the suite that owns the repo's only EQL v2 *and* v3 SQL coverage. So its `postgres-v3.test.ts` queried SQL installed from the npm bundle while the payloads under test were emitted by the in-tree `eql-bindings`: the two halves of EQL could disagree, and would have disagreed in a database rather than in CI. The directory is now a workspace member, `@cipherstash/eql` resolves `workspace:^`, and the registry-pin guard's exemption list is empty. Moving the pins was not neutral, which is why this was always meant to be its own change. `@cipherstash/auth` `^0.39.0 -> 0.42.0` is a breaking API move: `AccessKeyStrategy.create`, `OidcFederationStrategy.create` and `getToken` all return `@byteslice/result` Results now, and `OidcFederationStrategy.create` takes a whole CRN instead of `(region, workspaceId, getJwt)`. Four call sites needed unwrapping, and nothing would have caught them — the suite's `tsc` rode on its own `npm test` while CI invoked vitest directly, so it compiled nowhere. It has a `typecheck` script run from `tests.yml` now, with `checkJs` on so the two `.cjs` fixtures (one of them a real strategy call site) are covered too. Workspace membership has a cost worth naming: root `pnpm test` is `turbo test --filter './packages/**'`, which now reaches this package. The suite is kept out by naming no live script after a turbo task — its runners are `vitest:live` and `vitest:live:coverage`. `test` is the obvious trap and `test:integration` the less obvious one, being a real turbo task that four integration workflows invoke. `integrationSuiteCi.test.ts` derives the forbidden set from `turbo.json` rather than listing it, and also asserts `tasks.toml` still installs with pnpm, reading past comments so a sentence about `npm ci` cannot stand in for the command. No changeset: nothing in a published package's behaviour or surface changes. Verified with a credentialed local run against Docker and live ZeroKMS -- `mise run test:integration:all`, 20 files / 232 tests, zero skips, EQL v2 and v3 both installed. Also `pnpm test` (16/16 tasks, suite correctly not among them), `pnpm test:scripts` (818, stable over three runs), the e2e supply-chain suite, all four lint scripts, the release gate, and `pnpm install --frozen-lockfile` clean with one shared `@cipherstash/auth` instance across stack, cli, wizard and the suite. Refs #885 Claude-Session: https://claude.ai/code/session_01FGhktoHnwEuibNQF1MYexw
|
tobyhede
marked this pull request as ready for review
August 26, 2026 05:57
tobyhede
changed the title
freshtonic
approved these changes
Aug 31, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Approving. This executes the absorption AGENTS.md scheduled, and — the thing that paragraph said was the only acceptable proof — the credentialed integration suite ran on this PR's own head and passed (2m1s: secrets pre-flight → FFI binding build → Postgres + EQL v2/v3 provision → live native + WASM suite, every step green). So the pin changes are proven neutral in the only way they could be, not asserted.
Verified point by point:
- Pin deltas are accounted for.
@cipherstash/auth ^0.39.0 → catalog 0.42.0is the breaking one (Result-wrappedcreate/getToken, re-signaturedOidcFederationStrategy.create), and all four broken call sites are fixed and exercised by the passing live run — including the.cjsfixture that atests/**/*.tstypecheck scope would have missed, which the newcheckJstsconfig catches.@cipherstash/eql 3.0.2 → workspace:^(in-tree 3.0.5) is the point of the PR: the last registry pin of EQL is gone, so the SQL the suite installs and the Rust emitting payloads are the same commit. The droppedoverrides.viteis covered by the workspace's existing security override (lockfile resolves 7.3.6). - Every seam AGENTS.md predicts, moved: explicit workspace entry,
npm ci → pnpm install --frozen-lockfileintasks.toml(with a guard asserting exactly that), lockfile deleted, the eql-pins linter exemption retired to an empty map (forced by the linter's own stale-exemption exit-2, which is the mechanism working as designed), and the suite deliberately carries notestscript — with the new guard deriving the forbidden script names fromturbo.jsonrather than hardcoding them — so rootpnpm teststays cargo-free and Docker-free. - The typecheck hole that let the auth break land silently is closed: the suite's
tscnow runs at PR time intests.yml, and since protect-ffi'sbuildis tsc-only, this adds no cargo to that path. - Path filters extended correctly:
pnpm-workspace.yamlandpackages/eql/packages/eql/**join both filter copies, so a catalog bump or an EQL edit re-runs the suite it now feeds. - Meta files kept honest: AGENTS.md's "not a workspace member" paragraph rewritten, the linter-exemption paragraph updated, both absorption plan docs marked done and recording that the change was not neutral in code (four call sites) while being proven neutral in behaviour.
- Changeset correctly absent (private package, dev-only catalog addition, CI/guard/docs otherwise),
onlyBuiltDependenciesuntouched, and the vitest/coverage catalog lockstep got its own supply-chain test.
Two non-blocking observations:
- The old setup's stated virtue — the suite proving the published FFI surface works for a consumer resolving its own dependency tree — is gone with the lockfile; the workflow comment making that argument was deleted rather than rebutted. The e2e package-manager suites still cover consumer-shaped resolution, so this is fine, but one sentence acknowledging the trade where the comment used to live would keep the reasoning discoverable.
- Nit:
expect(turboTasks).toContain('test')on aSetworks under vitest 3 butturboTasks.has('test')(or an array) is sturdier against matcher behaviour changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CipherStash Stack encrypts data in the database. Two pieces have to agree for that to work: the Rust that produces an encrypted value, and the SQL (called EQL — Encrypt Query Language) that stores and searches it. They ship as a matched pair, at one version, because a mismatch is not a compile error — it is a query that returns wrong results, or nothing, at runtime in a customer's database.
Both pieces now live in this repository, so they can be built from the same commit and cannot disagree. One directory was still opting out:
packages/protect-ffi/integration-tests, the test suite that runs real encryption against a real Postgres. It sat outside the repo's package manager with a lockfile of its own, downloading a published copy of the EQL SQL from the npm registry while testing Rust built from this repo. That directory is also the only place the EQL SQL is exercised at all.So the one suite whose job is to catch that mismatch was the one place it could still happen. This PR brings the directory into the workspace, and the mismatch is now unrepresentable.
It was not a free move, which is why it was always planned as its own PR. Joining the workspace also means taking the repository's shared dependency versions, and one of those — the authentication library — had made breaking API changes across the versions being skipped. Those broke the suite, and are fixed here.
Changes
Joining the workspace
packages/protect-ffi/integration-testsis listed inpnpm-workspace.yaml; itspackage-lock.jsonis deleted and thenpm cistep is gone.@cipherstash/auth,vitest,typescript) come from the repo's catalog.Fixing what the version bump broke
@cipherstash/auth^0.39.0 → 0.42.0changed three functions to return a success/failure wrapper object instead of the value directly, and changed one function's arguments. Four call sites updated across three test files and a test fixture.Making the same class of break visible next time
typecheckscript, run on every pull request. Its type-check previously ran nowhere in CI, which is why a breaking API change could land silently..cjsfixtures. One is a real call site of the changed API, and the default file pattern would have skipped it.Keeping the ordinary test command usable
pnpm test, which every contributor runs. If one of its scripts were named after a task the build tool knows,pnpm testwould start demanding Docker, a database and live credentials for everyone. Its runners are namedvitest:live/vitest:live:coverageinstead, and a test derives the forbidden names fromturbo.jsonrather than listing them.Guards and docs
vitestand its coverage plugin stay on the same version — they refuse to run against each other otherwise.AGENTS.md, the package README, and both absorption planning documents updated.No changeset: no published package changes behaviour or public API here.
Verification
Run against Docker and live CipherStash credentials, with both EQL versions installed:
mise run test:integration:all— 20 files, 232 tests, 0 skipped. Includes the identity-bound encryption tests, both EQL v1-era and current SQL suites, and the WebAssembly round trip. Both shapes of the changed auth API are covered by a live test each.pnpm test— 16/16 tasks. The live suite is correctly not among them.pnpm test:scripts— 818 passing, stable across five runs.pnpm install --frozen-lockfileclean, with a single shared@cipherstash/authinstance acrossstack,cli,wizardand this suite — the arrangement whose absence caused the "Failed to load native binding" failures in 1.0.0-rc.2.Honest gaps:
npm ciappears nowhere in its log. It has not run on the default branch yet; that only happens after merge.pnpm test:scriptsfailed once (817/818) mid-work and passed on every run since. That run overlapped a concurrent build rewriting a file one of the tests reads, which I believe was the cause — but I did not capture the test name before it went green, so I cannot state that as fact.pnpm run code:checkcannot run repo-wide on my machine: stale agent worktrees under.claude/carry their own Biome config and abort it. Biome was run scoped to the changed paths instead — clean, warnings only.Related
Refs #885 — the EQL absorption this completes. That PR moved the SQL in-repo; this removes the last place a downloaded copy was still used.
Review notes
Start with
packages/protect-ffi/integration-tests/package.jsonandpnpm-workspace.yaml— everything else follows from those two.The auth API fixes are the part that could be wrong in a way tests would not catch, since they are test code themselves.
tests/js-strategy.test.tsis worth a close look: the API accepts two return shapes and the suite deliberately covers one per test. Before this change that split was accidental — the old library version returned the plain shape and the test forwarded it. Preserving the split took an explicit unwrap, and without it both tests would have exercised the same shape while the other, which is what hand-written integrations return, went untested.Deliberately kept:
@vitest/coverage-v8brings roughly fifteen transitive dev dependencies into the shared lockfile. It was already installed in the old isolated lockfile, where nothing monitored it — the absorption plan lists that as a problem this change resolves. Bringing it under the repo's dependency monitoring is the point, not a side effect.Not done, and I would rather it were raised than assumed: the
.cjsfixtures needed one type annotation each to pass the new type-check. That is a real constraint of checking plain JavaScript, not a workaround, and there is a comment saying so.https://claude.ai/code/session_01FGhktoHnwEuibNQF1MYexw